Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: port pyclient build config to pyproject.toml #1470

Merged
merged 12 commits into from
Jun 7, 2024
Merged

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Jun 5, 2024

Summary:

I hoped the more modern config's dynamic metadata would work, but it only works if __version__ is defined explicitly in __init__.py, not imported from delphi_epidata.py (importing delphi_epidata.py errors at build-time because the dependencies in that file aren't installed in the build environment (just setuptools)).

So, this doesn't come with a useful feature, it's just a lateral move to the more modern packaging config.

On the plus side, I learned a lot by reading parts of these guides:

Might be handy in the future when we do #939 and other such changes.

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

@dshemetov dshemetov requested a review from melange396 June 5, 2024 23:21
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great, i really like it! there are waaaaaay too many comments in here tho....

# https://packaging.python.org/en/latest/tutorials/packaging-projects/#choosing-a-build-backend
[build-system]
# A list of packages that are needed to build your package:
requires = ["setuptools>=61.0.0"] # REQUIRED if [build-system] table is used
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does that version get us? the sample/template pyproject.toml doesnt specify it

Copy link
Contributor Author

@dshemetov dshemetov Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea, it's not necessary here, I'll remove it. It's needed to support the dynamic version feature (see the callout box in bullet 1 here "As of the release of setuptools 61.0.0..."). I used it in delphi-logger though and it works!

src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
src/client/packaging/pypi/pyproject.toml Outdated Show resolved Hide resolved
@dshemetov dshemetov requested a review from melange396 June 6, 2024 18:05
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, except i dont think this line is going to work any more:

python setup.py sdist bdist_wheel

which probably also means updating this line:

and we should put an entry in https://github.com/cmu-delphi/delphi-epidata/blob/dev/src/client/packaging/pypi/CHANGELOG.md

Copy link

sonarqubecloud bot commented Jun 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dshemetov
Copy link
Contributor Author

dshemetov commented Jun 7, 2024

Good point, thanks! I swapped the commands over to the new ones (sdist and wheel) and added the new build dependency to CI.

Also updated the publish action based on hints from my IDE:

image

And updated changelog too (I figure we can just keep the changelog dates approximate? Doesn't feel like a big deal if those are one day off).

@dshemetov dshemetov requested a review from melange396 June 7, 2024 00:10
@melange396
Copy link
Collaborator

we can always fix the date on release day

@melange396 melange396 merged commit e423cb4 into dev Jun 7, 2024
7 checks passed
@melange396 melange396 deleted the ds/py-client branch June 7, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants